Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remote function planning #14718

Merged
merged 4 commits into from
Jul 16, 2020
Merged

Remote function planning #14718

merged 4 commits into from
Jul 16, 2020

Conversation

rongrong
Copy link
Contributor

@rongrong rongrong commented Jun 24, 2020 •
edited
Loading

This PR introduced the concept of "remote function" into planner and generates query plan that's aware of the remote functions. Depends on #14219. Partially addresses #14053.

Ran verifier explain tests across all dcs and all tests passed.

== NO RELEASE NOTE ==

@rongrong rongrong force-pushed the remote-function-planning branch 2 times, most recently from f21530d to cd20f3b Compare June 25, 2020 01:24
highker
highker reviewed Jun 26, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Add rule to rewrite filter with remote function to project" LGTM

@highker highker self-requested a review June 26, 2020 19:56
caithagoras
caithagoras reviewed Jul 6, 2020
View reviewed changes
Copy link
Contributor

@caithagoras caithagoras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the 1st and the 2nd commit.

...n/java/com/facebook/presto/functionNamespace/AbstractSqlInvokedFunctionNamespaceManager.java Show resolved Hide resolved
.../src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestPlanRemoteProjections.java Outdated Show resolved Hide resolved
.../src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestPlanRemoteProjections.java Outdated
public TestPlanRemoteProjections()
{
FunctionManager functionManager = METADATA.getFunctionManager();
functionManager.addTestFunctionNamespace("unittest", new InMemoryFunctionNamespaceManager("unittest", new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql,java")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use H2 FNM, which is designed for testing purpose. We can then avoid adding test-related method addTestFunctionNamespace in our production code.

You can do that by first adding the factory:

static {
    METADATA.getFunctionManager().addFunctionNamespaceFactory(new H2FunctionNamespaceManagerFactory());
}

You'll then be able to add a function namespace by

functionManager.loadFunctionNamespaceManager(
    H2FunctionNamespaceManagerFactory.NAME,
    "unittest",
    ImmutableMap.of(<config key>, <config value>))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember I tried that but it didn't work. But I don't remember what didn't work. This is the problem of delayed review... Let me try again I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason to not use H2FunctionNamespaceManagerFactory is dependencies. This test is in presto-main. H2FunctionNamespaceManagerFactory is in presto-tests.

caithagoras
caithagoras reviewed Jul 6, 2020
View reviewed changes
Copy link
Contributor

@caithagoras caithagoras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3rd commit.

presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java
{
List<FunctionCall> externalFunctions = extractExternalFunctions(functionHandles, ImmutableList.of(predicate), functionManager);
if (!externalFunctions.isEmpty()) {
throw new SemanticException(NOT_SUPPORTED, predicate, "External functions in %s is not supported: %s", clause, externalFunctions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, for better readability of the constructed error message:

External functions in [%s] is not supported: %s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no unit test coverage for this commit. Can we add some.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, for better readability of the constructed error message:

External functions in [%s] is not supported: %s

I don't think the [] is useful. It would make the error message like this:

External functions in [Lambda expression] is not supported:....

presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionTreeUtils.java Outdated Show resolved Hide resolved
@rongrong rongrong force-pushed the remote-function-planning branch from cd20f3b to cd2775c Compare July 13, 2020 23:11
@rongrong rongrong requested a review from caithagoras July 13, 2020 23:11
@rongrong rongrong force-pushed the remote-function-planning branch 3 times, most recently from ef66bcd to 7b3e57c Compare July 14, 2020 00:28
@rongrong rongrong force-pushed the remote-function-planning branch from 7b3e57c to 5a488ba Compare July 15, 2020 20:24
highker
highker reviewed Jul 15, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Add Locality to ProjectNode" LGTM

presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/VerifyProjectionLocality.java Show resolved Hide resolved
highker
highker reviewed Jul 15, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Explicitly not supporting external functions in lambda and join filter" LGTM

@kaikalur
Copy link
Contributor

Overall, looks good. It would be good to add a few tests that actually show a real plan string (like explain output) with external functions so the logic becomes clearer and will help future iterations on this feature.

@rongrong rongrong force-pushed the remote-function-planning branch from 5a488ba to f06cf7b Compare July 15, 2020 20:58
highker
highker approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

presto-main/src/main/java/com/facebook/presto/metadata/FunctionManager.java Outdated Show resolved Hide resolved
kaikalur
kaikalur approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted offline about testing the plans - will wait for the execution PR.

LGTM

rongrong added 4 commits July 15, 2020 15:12
@rongrong
Add Locality to ProjectNode
d4ac04a
@rongrong
Plan projection with remote functions
fac8b4e
@rongrong
Add rule to rewrite filter with remote function to project
846acce
@rongrong
Explicitly not supporting external functions in lambda and join filter
b460e10 
We check the filter predicate in JOIN in PlanSanityChecker so we can cover
correlated IN and lateral join that are converted to JOIN with filter predicate.
@rongrong rongrong force-pushed the remote-function-planning branch from f06cf7b to b460e10 Compare July 15, 2020 22:15
@rongrong rongrong merged commit f5868d4 into prestodb:master Jul 16, 2020
@rongrong rongrong deleted the remote-function-planning branch July 16, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caithagoras caithagoras caithagoras approved these changes

@highker highker highker approved these changes

@kaikalur kaikalur kaikalur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rongrong @kaikalur @highker @caithagoras